chore: error-handling convention rollout (#1738)#1756
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (3)src/synthorg/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (4)
WalkthroughThe commit migrates many exception classes under src/synthorg/ to DomainError-rooted families and adds ClassVar metadata (default_message, error_category, error_code, status_code). It introduces an AST-based gate scripts/check_domain_error_hierarchy.py with a frozen baseline file and a local pre-commit/pre-push hook, and documents the gate in docs/reference/errors.md and docs/reference/conventions.md. Several controllers now raise domain exceptions (NotFoundError, ValidationError) instead of returning Response objects. VersionConflictError was replaced by PersistenceVersionConflictError and corresponding imports, tests, and persistence repos were updated. |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a standardized domain error hierarchy and an AST-based pre-commit gate to enforce it. The changes include a new validation script, updated documentation, and the migration of numerous exception classes to inherit from DomainError. Additionally, persistence-layer conflict errors were renamed to avoid naming collisions. Review feedback identified a critical Python 3 syntax error in the gate script, a logic error in how imports are resolved, and an issue with non-recursive file scanning.
| capture_output=True, | ||
| cwd=project_root, | ||
| ) | ||
| except subprocess.CalledProcessError, FileNotFoundError: |
There was a problem hiding this comment.
This line contains a syntax error in Python 3. The comma-separated syntax for catching multiple exceptions (except E1, E2:) was removed in favor of tuples (except (E1, E2):). Furthermore, in Python 3, the comma syntax is interpreted as except E1 as E2:, meaning this code currently catches CalledProcessError and renames it to FileNotFoundError, while failing to catch actual FileNotFoundError exceptions (which occur if git is not installed).
| except subprocess.CalledProcessError, FileNotFoundError: | |
| except (subprocess.CalledProcessError, FileNotFoundError): |
| aliases[alias.asname] = (alias.name, None) | ||
| else: | ||
| head = alias.name.split(".")[0] | ||
| aliases.setdefault(head, (alias.name, None)) |
There was a problem hiding this comment.
This logic incorrectly maps the root package name to the full imported module path when no alias is used. In Python, import a.b.c adds a to the local namespace, not a.b.c. This causes the AST resolver to incorrectly compute the module path for base classes in files using non-aliased imports (e.g., it would resolve synthorg.core.domain_errors.DomainError as synthorg.core.domain_errors.core.domain_errors.DomainError).
aliases.setdefault(head, (head, None))| rel_root = abs_root.relative_to(project_root).as_posix() or "." | ||
| try: | ||
| result = subprocess.run( | ||
| ["git", "ls-files", "-z", "--", f"{rel_root}/*.py"], |
There was a problem hiding this comment.
The current git ls-files command uses a shell-style glob *.py which is not recursive in this context. It will only find Python files in the immediate directory specified by rel_root, skipping all subdirectories (e.g., src/synthorg/core/). To ensure the gate scans the entire tree, you should pass the directory path to git and filter by extension in Python.
["git", "ls-files", "-z", "--", rel_root],| for p in sorted(abs_root.rglob("*.py")) | ||
| ] | ||
| out = result.stdout.decode("utf-8", errors="replace") | ||
| paths = [p for p in out.split("\0") if p] |
There was a problem hiding this comment.
Since the git ls-files command should be updated to return all tracked files in the directory to ensure recursion, you must filter the results here to only include Python files.
paths = [p for p in out.split("\0") if p and p.endswith(".py")]
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/synthorg/backup/errors.py (1)
59-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackupUnrestartableError is outside the BackupError family contract.
Line 3 says all backup-related errors inherit
BackupError, but Line 59 inheritsConflictErrordirectly. That meansexcept BackupErrorwill not catch this class.💡 Proposed fix
-from synthorg.core.domain_errors import ConflictError, DomainError +from synthorg.core.domain_errors import DomainError from synthorg.core.error_taxonomy import ErrorCategory, ErrorCode @@ -class BackupUnrestartableError(ConflictError): +class BackupUnrestartableError(BackupError): @@ default_message: ClassVar[str] = ( "Backup scheduler is unrestartable after a timed-out stop" ) + error_category: ClassVar[ErrorCategory] = ErrorCategory.CONFLICT + error_code: ClassVar[ErrorCode] = ErrorCode.RESOURCE_CONFLICT + status_code: ClassVar[int] = 409🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/backup/errors.py` around lines 59 - 69, The BackupUnrestartableError class currently subclasses ConflictError directly which breaks the BackupError family contract; change its base class to BackupError (ensuring BackupError itself still maps to ConflictError as intended) so that "except BackupError" will catch BackupUnrestartableError; keep the existing default_message and docstring intact and update the class definition for BackupUnrestartableError to inherit from BackupError instead of ConflictError.src/synthorg/meta/appliers/code_applier.py (1)
432-434:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
_write_changesRaisesdocstring to match actual exception type.Line 433 still documents
RuntimeError, but this path now wraps failures inPartialWriteError. The stale contract can mislead callers and tests.Proposed docstring fix
Raises: - RuntimeError: If a file write or delete fails. + PartialWriteError: If applying a change fails after one or more + changes were written; carries the applied subset for rollback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/meta/appliers/code_applier.py` around lines 432 - 434, The _write_changes docstring incorrectly documents that it raises RuntimeError; update the Raises section to list PartialWriteError (the actual exception type now used when file write/delete fails) and adjust any explanatory text to reference PartialWriteError and its semantics; locate and modify the _write_changes method's docstring to replace "RuntimeError" with "PartialWriteError" and ensure the description matches the wrapped failure behavior.src/synthorg/hr/errors.py (1)
17-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't default the whole HR family to
INTERNAL_ERROR/500.
HRErrornow makes every subtype without an override serialize as a generic 500. In this file that newly affects business-state exceptions likeHiringApprovalRequiredError,HiringRejectedError, andPromotionApprovalRequiredError, which are not server faults. Give those subtypes explicit 4xx metadata or move them under a more specific semantic base beforehandle_domain_errorstarts reading these ClassVars.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/hr/errors.py` around lines 17 - 29, HRError currently sets error_category=ErrorCategory.INTERNAL and status_code=500 so any subclass that doesn't override those ClassVars will serialize as a 500; update the business-state exceptions (e.g., HiringApprovalRequiredError, HiringRejectedError, PromotionApprovalRequiredError) to either inherit from a new semantic base (e.g., HRClientError) that sets error_category=ErrorCategory.CLIENT, error_code=ErrorCode.INVALID_REQUEST (or appropriate 4xx code) and status_code=400/403 as applicable, or explicitly set those ClassVars on each subclass, and ensure handle_domain_error will read the new ClassVars for correct 4xx responses.src/synthorg/persistence/postgres/workflow_execution_repo.py (1)
228-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDifferentiate delete races from true version conflicts.
rowcount == 0here means either “version mismatch” or “row no longer exists”. RaisingPersistenceVersionConflictErrorfor both collapses a delete race into a 409 conflict and loses the not-found path. Please probe for the current row/version before choosing the exception, and keep the SQLite backend aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/persistence/postgres/workflow_execution_repo.py` around lines 228 - 239, The save/update path that currently treats cur.rowcount == 0 as a blanket PersistenceVersionConflictError should distinguish a deleted row from a real version mismatch: after detecting rowcount == 0 in the save function (the method around where PERSISTENCE_WORKFLOW_EXEC_SAVE_FAILED and PersistenceVersionConflictError are used), run a SELECT version FROM workflow_executions WHERE id = <execution.id> to see if the row exists; if no row is returned raise the repository's not-found error (aligning with the SQLite backend behavior), otherwise raise PersistenceVersionConflictError and include the current version value in the error message/log so callers can see the true version mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 277-287: The pre-commit hook "domain-error-hierarchy" is only
registered for the pre-push stage; update the .pre-commit-config.yaml entry for
id "domain-error-hierarchy" to include the pre-commit stage as well (e.g.,
change the stages value from [pre-push] to include pre-commit such as
[pre-commit, pre-push]) so the gate runs at commit time as required by the
rollout objective.
In `@scripts/check_domain_error_hierarchy.py`:
- Around line 377-388: The rooted short-circuit currently skips entries as soon
as they reach DomainError, allowing mixed inheritance like class
Foo(DomainError, ValueError) to bypass checks; move or add the call to
_has_forbidden_direct_base(entry) before the rooted membership check (the loop
over all_entries that uses variables entry, rooted, and key) so that you first
test _has_forbidden_direct_base(entry) and only then apply the rooted fast-path;
apply the same change in the second scan block (the analogous code around the
482-489 region) so forbidden direct bases are reported unless explicitly
allowlisted.
In `@src/synthorg/api/cursor.py`:
- Around line 27-37: The docstring for InvalidCursorError is out of sync: it
says controllers should translate this to HTTP 400 but the class sets
status_code = 422; update the docstring to reflect the implemented status (HTTP
422 Unprocessable Entity) and any guidance on controller mapping so it matches
the status_code ClassVar on InvalidCursorError and the error_category/error_code
metadata.
In `@src/synthorg/api/services/workflow_rollback_service.py`:
- Around line 9-13: The service docstring in the workflow_rollback_service
module includes controller/HTTP-level translation details (mentions durable
definition save, PersistenceVersionConflictError,
synthorg.core.domain_errors.VersionConflictError, RFC 9457 and "produces a 409")
which should be removed; edit the module- and function-level docstrings to keep
only layer-local contract and behavior (e.g., what exceptions the service itself
raises such as PersistenceVersionConflictError) and delete any references to
controllers, HTTP mapping, or RFC/error-code dispatching so the docstring stays
focused on the service's local responsibilities.
In `@src/synthorg/engine/strategy/principles.py`:
- Around line 294-298: The try/except around ConstitutionalPrinciple(**raw)
currently raises StrategyPackValidationError without logging; before raising,
log the validation failure at WARNING (including index i, the raw payload, and
the exception message) or emit the existing strategy-pack failure event so
malformed config.custom entries surface; add a logger.warning(...) or call to
the module's strategy-pack failure event helper with context (i, raw, exc)
immediately inside the except block, then raise StrategyPackValidationError as
before.
In `@tests/unit/api/controllers/test_setup_locales.py`:
- Around line 268-271: Replace the three bare AsyncMock assignments to
settings_svc.get_entry with AsyncMock that include a concrete spec;
specifically, when you reassign settings_svc.get_entry (the three occurrences
using AsyncMock with side_effect SettingNotFoundError(...)), pass
spec=settings_svc.__class__ (or the concrete SettingsService class) to AsyncMock
so the mock adheres to the real service interface; update all three locations
that currently call AsyncMock(...) to AsyncMock(spec=settings_svc.__class__,
...).
In `@tests/unit/api/controllers/test_setup.py`:
- Around line 1208-1211: Replace the ad-hoc AsyncMock assignments for
settings_svc.get_entry with AsyncMock(..., spec=original) (use the saved
original = settings_svc.get_entry) so each mock declares a spec; update both
places that assign settings_svc.get_entry (the AsyncMock that raises
SettingNotFoundError and the other occurrence around line ~1256) to pass
spec=original to the AsyncMock constructor to comply with the test mocking rule.
In `@tests/unit/api/controllers/test_workflow_versions.py`:
- Around line 347-356: Move the block-local imports in the test
test_rollback_persistence_version_conflict_translates_to_409 into module scope:
hoist imports for WorkflowRollbackService, ErrorCategory, ErrorCode, and
PersistenceVersionConflictError out of the test body to the top of the test
module so the test function body falls under the 50-line limit; keep the exact
imported names and do not change any behavior or references inside the test.
In `@tests/unit/api/test_exception_handlers.py`:
- Around line 1640-1647: Remove the explicit issue back-reference and
migration-framing language from the test docstring and nearby comment blocks
(the docstring containing "End-to-end ClassVar -> HTTP mapping for the `#1738`
migration" and the similar text at the other location), and replace with a
concise, test-focused explanation of what the test validates (e.g., "Verify
ClassVar->HTTP metadata mapping and that typos in
status_code/error_code/error_category are caught for DomainError subclasses").
Update the docstrings/comments that reference "#1738", "migration", or similar
review/issue context so they only describe the test's purpose and expectations;
preserve mentions of the problematic attributes (status_code, error_code,
error_category) and the fact the test catches typos. Ensure changes touch the
module/test docstring and the other comment block around the same test (the
blocks referenced above).
In `@tests/unit/scripts/test_check_domain_error_hierarchy.py`:
- Line 416: The tests currently call monkeypatch.chdir(project_root) which masks
bugs where main() ignores the --repo-root flag; change those calls so the
current working directory is NOT the repo_root under test (e.g., remove
monkeypatch.chdir(project_root) or set CWD to a different temp dir such as
tmp_path or project_root.parent) so the CLI is forced to resolve paths from the
provided --repo-root; update all occurrences of monkeypatch.chdir(project_root)
(the calls at the three locations noted) and ensure the test invokes main() with
the --repo-root argument pointing to project_root so the flag contract is
actually exercised.
---
Outside diff comments:
In `@src/synthorg/backup/errors.py`:
- Around line 59-69: The BackupUnrestartableError class currently subclasses
ConflictError directly which breaks the BackupError family contract; change its
base class to BackupError (ensuring BackupError itself still maps to
ConflictError as intended) so that "except BackupError" will catch
BackupUnrestartableError; keep the existing default_message and docstring intact
and update the class definition for BackupUnrestartableError to inherit from
BackupError instead of ConflictError.
In `@src/synthorg/hr/errors.py`:
- Around line 17-29: HRError currently sets
error_category=ErrorCategory.INTERNAL and status_code=500 so any subclass that
doesn't override those ClassVars will serialize as a 500; update the
business-state exceptions (e.g., HiringApprovalRequiredError,
HiringRejectedError, PromotionApprovalRequiredError) to either inherit from a
new semantic base (e.g., HRClientError) that sets
error_category=ErrorCategory.CLIENT, error_code=ErrorCode.INVALID_REQUEST (or
appropriate 4xx code) and status_code=400/403 as applicable, or explicitly set
those ClassVars on each subclass, and ensure handle_domain_error will read the
new ClassVars for correct 4xx responses.
In `@src/synthorg/meta/appliers/code_applier.py`:
- Around line 432-434: The _write_changes docstring incorrectly documents that
it raises RuntimeError; update the Raises section to list PartialWriteError (the
actual exception type now used when file write/delete fails) and adjust any
explanatory text to reference PartialWriteError and its semantics; locate and
modify the _write_changes method's docstring to replace "RuntimeError" with
"PartialWriteError" and ensure the description matches the wrapped failure
behavior.
In `@src/synthorg/persistence/postgres/workflow_execution_repo.py`:
- Around line 228-239: The save/update path that currently treats cur.rowcount
== 0 as a blanket PersistenceVersionConflictError should distinguish a deleted
row from a real version mismatch: after detecting rowcount == 0 in the save
function (the method around where PERSISTENCE_WORKFLOW_EXEC_SAVE_FAILED and
PersistenceVersionConflictError are used), run a SELECT version FROM
workflow_executions WHERE id = <execution.id> to see if the row exists; if no
row is returned raise the repository's not-found error (aligning with the SQLite
backend behavior), otherwise raise PersistenceVersionConflictError and include
the current version value in the error message/log so callers can see the true
version mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64d5a9db-3d99-46ad-8988-c0a5577bd899
📒 Files selected for processing (78)
.pre-commit-config.yamlCLAUDE.mddocs/reference/conventions.mddocs/reference/errors.mdscripts/check_domain_error_hierarchy.pyscripts/domain_error_hierarchy_baseline.txtsrc/synthorg/a2a/client.pysrc/synthorg/a2a/gateway.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/cursor.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/backup/errors.pysrc/synthorg/client/factory.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/config/errors.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/core/registry/errors.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/hr/errors.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/appliers/_validation.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/meta/errors.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/security/trust/errors.pysrc/synthorg/settings/errors.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/tools/html_parse_guard.pytests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_setup.pytests/unit/api/controllers/test_setup_locales.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/api/fakes_workflow.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/api/test_exception_handlers.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_service.pytests/unit/meta/test_code_applier.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/scripts/test_check_domain_error_hierarchy.py
💤 Files with no reviewable changes (1)
- src/synthorg/api/exception_handlers.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Backend
- GitHub Check: Lighthouse Site
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
No
from __future__ import annotations: Python 3.14 has PEP 649.PEP 758 except:
except A, B:(no parens) when not binding;as excrequires parens.Type hints: all public functions; mypy strict.
Docstrings: Google style on public classes / functions (ruff D rules).
Never mutate; create new objects via
model_copy(update=...)orcopy.deepcopy(). Frozen Pydantic for config/identity;MappingProxyTypefor non-Pydantic registries; deepcopy at system boundaries (tool execution, provider serialization, persistence).Separate frozen config models from mutable-via-copy runtime models; never mix in one model.
Pydantic v2:
ConfigDict(frozen=True, allow_inf_nan=False)everywhere;extra="forbid"on every model that doesn't round-trip throughmodel_dump()(request DTOs always);@computed_fieldfor derived values;NotBlankStrfromcore.typesfor identifier / name fields.Args models at every system boundary (
BaseTool, MCP tool, A2A RPC, WebSocket event): typed Pydantic args model validated before dispatch.Typed-boundary helper: every dict ingestion from an external source (MCP args, JWT decode, WebSocket control, audit-chain payload, A2A JSON-RPC, settings security import) calls
parse_typed()fromsynthorg.api.boundarywith a hardcodedLiteralStringboundarylabel.Async concurrency: prefer
asyncio.TaskGroupfor fan-out / fan-in; wrap independent task bodies inasync defhelpers that catchException(re-raise onlyMemoryError/RecursionError).Clock seam: classes that read time or sleep take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClock.Lifecycle sync: async
start()/stop()services own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartable.Untrusted-content fences (SEC-1): wrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safety; appenduntrusted_content_directive(tags).HTML parsing (SEC-1): never call `lxml.html.f...
Files:
tests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/api/cursor.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/api/controllers/test_setup.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/security/trust/errors.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/errors.pytests/unit/core/test_persistence_errors.pytests/unit/engine/workflow/test_execution_service.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/budget_config_versions.pytests/unit/api/controllers/test_setup_locales.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/meta/appliers/_validation.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/client/factory.pytests/unit/api/test_exception_handlers.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/meta/appliers/code_applier.pytests/unit/meta/test_code_applier.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/meta/appliers/github_client.pytests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pytests/unit/api/fakes_workflow.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/a2a/client.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/hr/errors.pyscripts/check_domain_error_hierarchy.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/settings/errors.pysrc/synthorg/persistence/workflow_execution_repo.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.pytests/unit/engine/workflow/test_execution_lifecycle.pysrc/synthorg/core/registry/errors.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/backup/errors.pysrc/synthorg/core/persistence_errors.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass.Use
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)) for shared mocks.Import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter for time-driven tests.FakeClock.sleepadvances virtual time and yields once viaasyncio.sleep(0).Never use real vendor names in test files; use
test-provider,test-small-001.Comments explain WHY only in test files; no review citations, issue back-refs, or migration framing.
Test markers:
@pytest.mark.unit/integration/e2e/slow.Async:
asyncio_mode = "auto"; no manual@pytest.mark.asyncio.Timeout: 30s per test (global in
pyproject.toml); don't add per-filetimeout(30)markers; non-default liketimeout(60)is allowed.Never
monkeypatch.setattr(module.logger, "info", spy)for logger spying; theBoundLoggerLazyProxycaches the stale bound method via__dict__. Usetry/finally del proxy.<level>instead.Parametrize: prefer
@pytest.mark.parametrizefor similar cases.Property-based: Hypothesis (Python), fast-check (React),
testing.F(Go). CI runs 10 deterministic examples (derandomize=True). Hypothesis failures are real bugs: fix the bug and add an@example(...)decorator.Flaky tests: NEVER skip/xfail/dismiss; fix fundamentally. FakeClock-first when the class accepts
clock=. For "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large).
Files:
tests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/api/controllers/test_setup.pytests/unit/core/test_persistence_errors.pytests/unit/engine/workflow/test_execution_service.pytests/unit/api/controllers/test_setup_locales.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/api/test_exception_handlers.pytests/unit/meta/test_code_applier.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/api/fakes_workflow.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.pytests/unit/engine/workflow/test_execution_lifecycle.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/api/controllers/test_setup.pytests/unit/core/test_persistence_errors.pytests/unit/engine/workflow/test_execution_service.pytests/unit/api/controllers/test_setup_locales.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/api/test_exception_handlers.pytests/unit/meta/test_code_applier.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/api/fakes_workflow.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.pytests/unit/engine/workflow/test_execution_lifecycle.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle errors explicitly, never swallow. Domain error families register a base-class entry in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py). Use<Domain><Condition>Errorinheriting fromDomainError; any ofException/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErroras a direct base insrc/synthorg/is forbidden.Every business-logic module:
from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Variable name alwayslogger. Carve-outs documented in module docstring.Never
import logging/logging.getLogger()/print()in application code (carve-out:observability/{setup,sinks,*_handler}.pyfor handler bootstrap).Event names: import constants from
synthorg.observability.events.<domain>; never string literals.Structured kwargs:
logger.info(EVENT, key=value); neverlogger.info("msg %s", val).Error paths log at WARNING or ERROR with context before raising / returning.
State transitions log INFO via
*_STATUS_TRANSITIONEDconstants (withfrom_status/to_status/ domain id) AFTER the persistence write succeeds.DEBUG for object creation, internal flow, key entry/exit. Pure data models, enums, re-exports skip logging.
Secret-log redaction (SEC-1): never call any
loggerseverity witherror=str(exc); useerror_type=type(exc).__name__anderror=safe_error_description(exc).Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code/tests/comments/docstrings/configs. Use
example-provider,example-{large,medium,small}-001. Allowed in:.claude/files, third-party import paths,src/synthorg/providers/presets.py(user-facing runtime data),web/public/provider-logos/*.svg. Tests usetest-provider,test-small-001.Comments explain WHY only, never origin / review / issue context. Forbidden in source / tests / docstrings / commit bodies: review...
Files:
src/synthorg/api/cursor.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/security/trust/errors.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/errors.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/meta/appliers/_validation.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/client/factory.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/a2a/client.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/hr/errors.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/settings/errors.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/core/registry/errors.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/backup/errors.pysrc/synthorg/core/persistence_errors.py
src/synthorg/**/{api,controllers,services}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Controllers and API endpoints access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService); services centralize audit logging; repositories must not log mutations themselves.
Files:
src/synthorg/api/cursor.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/auth/service.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s). Revalidation failures tracked via_SlidingWindowRateLimiter(api.ws_revalidation_window_seconds60s,api.ws_revalidation_max_failures5); saturation closes the socket with code 4011.
Files:
src/synthorg/api/cursor.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/auth/service.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/api/cursor.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/security/trust/errors.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/errors.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/meta/appliers/_validation.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/client/factory.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/a2a/client.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/hr/errors.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/settings/errors.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/core/registry/errors.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/backup/errors.pysrc/synthorg/core/persistence_errors.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines. Use markdown tables for tabular data; never usetextfences with ASCII box-drawing.
Files:
docs/reference/errors.mddocs/reference/conventions.md
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Datetime in persistence:
parse_iso_utc/format_iso_utcfromsynthorg.persistence._shared(both reject naive);normalize_utcfor relaxed coercion on already-typeddatetime.
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DML. Every durable feature defines a Protocol inpersistence/<domain>_protocol.py+ concrete impls underpersistence/{sqlite,postgres}/exposed onPersistenceBackend.
Files:
src/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/workflow_execution_repo.py
src/synthorg/meta/mcp/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define
ToolHandlerinsrc/synthorg/meta/mcp/handlers/<domain>.py, declareargs_model, callrequire_destructive_guardrails(arguments, actor)on anyadmin_tool, route through service-layer facades (neverapp_state.persistence.*directly), emit the three log paths viacommon_logginghelpers.
Files:
src/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/mcp/handlers/approvals.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Always read the relevant `docs/design/` page before implementing or planning. Deviations require explicit user approval; update the design page when an approved deviation lands. Never silently diverge.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding. Be critical at every phase; surface improvements as suggestions, not silent changes. Prioritize by dependency order, not priority labels.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: D2 diagram configuration uses theme 200 (Dark Mauve), dark-only, configured in `mkdocs.yml`; CI pins D2 CLI to v0.7.1 in `.github/workflows/pages.yml`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Validate at system boundaries (user input, external APIs, config files).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Never call `cd` in Bash commands; cwd is already project root. Exception: `bash -c "cd <dir> && <cmd>"` is safe (child process). Use this for tools without `-C`, e.g. `bash -c "cd web && npm install"`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Never use Bash to write files: use Write or Edit. Forbidden: `cat >`, `cat << EOF`, `echo >`, `echo >>`, `sed -i`, `python -c "open(...).write(...)"`; read-only piping to stdout is fine.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Signed commits: required on every protected ref. GPG/SSH signed, OR GitHub App-signed via the `synthorg-repo-bot`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Branches: `<type>/<slug>` from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Merge strategy: squash. PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in the PR body to land.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push. Do NOT auto-create a PR. ALWAYS use `/pre-pr-review` to create PRs (`gh pr create` is hookify-blocked). Trivial / docs-only: `/pre-pr-review quick`. After the PR exists, `/aurelio-review-pr` handles external reviewer feedback.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes. No deferring, no "out of scope".
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: When tests fail due to timeout / slowness / xdist contention: NEVER delete, skip, or `xfail`; NEVER `--no-verify`; NEVER edit `tests/baselines/unit_timing.json`. Suite time exceeding `baseline * 1.3` is a source-code regression; fix the source, not the tests.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Coverage: 80% minimum (CI; benchmarks excluded).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Parallelism: `pytest-xdist -n 8 --dist=loadfile` (always). `loadfile` prevents the cumulative resource leak `worksteal` triggers on Python 3.14 + Windows ProactorEventLoop.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:05:49.459Z
Learning: Isolation regression gate: `scripts/run_affected_tests.py` re-runs the affected subset twice via `pytest-repeat --count 2 -x` after the green pass. Opt out via `SYNTHORG_SKIP_ISOLATION_GATE=1`.
🔇 Additional comments (49)
src/synthorg/meta/appliers/config_applier.py (1)
49-51: Looks good — explicit lint carve-out is correctly scoped.The private
_PathResolutionErrorkeeps intentional stdlib inheritance and documents the exception to the hierarchy gate inline, which matches the rollout pattern.src/synthorg/meta/mcp/handlers/approvals.py (2)
66-68: Handler-local not-found sentinel is correctly scoped and documented.Using a private sentinel with explicit
domain_codeplus a per-line allowlist is appropriate here for MCP envelope mapping without HTTP-layer coupling.
80-82: Conflict sentinel change is clean and consistent with the MCP error-envelope design.Private, handler-local conflict signaling with explicit lint allow is a good fit for this path.
src/synthorg/api/auth/ticket_store.py (2)
49-55: Domain-error migration is correctly implemented.
TicketLimitExceededErrornow inherits fromPerOperationRateLimitErrorand declares the requiredClassVarmetadata for centralized RFC 9457 mapping.
173-177: 429 path is PII-safe and handler-friendly.This keeps identifying detail in server logs while raising a typed domain error that can render a generic response from class defaults.
src/synthorg/meta/appliers/github_client.py (1)
28-58: Domain-error migration here looks correct and coherent.The
IntegrationErrorinheritance plus thegithub_status_codefield separation and auth taxonomyClassVars are aligned with centralized error mapping behavior.tests/unit/meta/test_code_applier.py (1)
440-443: Assertion update is correct for the renamed exception field.This test now validates the intended
GitHubAPIError.github_status_codecontract.src/synthorg/backup/errors.py (1)
16-23: Good RFC 9457 metadata normalization across backup errors.The
ClassVarmappings onBackupError,BackupInProgressError, andBackupNotFoundErrorare clear and consistent with centralized domain-error dispatch.Also applies to: 28-31, 53-56
src/synthorg/persistence/postgres/settings_repo.py (1)
34-36: LGTM — internal sentinel exception correctly renamed and scoped.The renaming from
_CASConflictto_CASConflictErroris complete and consistent. The class correctly uses the stdlibExceptionbase with the requiredlint-allowcomment, and the docstring accurately describes it as an internal sentinel that never escapes the repository. The CAS control-flow pattern (raise inside transaction → rollback → catch → returnFalse) is correct.Also applies to: 342-343, 363-364
src/synthorg/settings/errors.py (1)
9-34: DomainError metadata migration looks consistent.
SettingsError,SettingNotFoundError, andSettingValidationErrornow align with centralized RFC 9457-style mapping viaClassVarmetadata and aDomainErrorroot.tests/unit/api/controllers/test_company.py (1)
109-113: 404 expectation is correct for this migrated error path.Updating the assertion on Line 112 to
404aligns this test with theSettingNotFoundErrordomain mapping.src/synthorg/meta/rollout/inverse_dispatch.py (1)
31-33: Looks good — sentinel exception is explicitly allowlisted and unchanged semantically.src/synthorg/persistence/workflow_execution_repo.py (1)
29-29: Docstring update is correct and aligned with the renamed persistence conflict error.src/synthorg/meta/telemetry/emitter.py (1)
54-56: Approved — retry sentinel remains intentionally stdlib-rooted with explicit lint exception.src/synthorg/tools/html_parse_guard.py (1)
73-75: Looks good — explicit allowlist marker is present for the internal XXE sentinel exception.src/synthorg/api/controllers/role_versions.py (1)
18-19: Approved — not-found flow is correctly converted toNotFoundErrorwith warning context prior to raising.Also applies to: 94-96
src/synthorg/api/controllers/budget_config_versions.py (1)
19-20: Looks good — consistent migration to exception-based 404 handling viaNotFoundError.Also applies to: 95-97
src/synthorg/api/controllers/company_versions.py (1)
19-20: Approved — the not-found path now correctly relies on centralized domain exception handling.Also applies to: 95-97
src/synthorg/observability/audit_chain/tsa_client.py (1)
71-73: Looks good — explicit lint allow is correctly attached to the internal TSA error root.src/synthorg/api/controllers/evaluation_config_versions.py (1)
18-18: Controller not-found flow is correctly migrated to domain exceptions.The warning log +
NotFoundErrorraise path is consistent with centralized RFC 9457 handling and keeps the endpoint behavior clean.Also applies to: 95-96
src/synthorg/meta/rollout/regression/welch.py (1)
30-32: Sentinel exception allowlist annotations look correct and explicit.The inline
lint-allowreasons are clear and keep the hierarchy gate intent auditable.Also applies to: 36-38
src/synthorg/persistence/sqlite/backup_utils.py (1)
50-52:IntegrityCheckErrorsentinel declaration update is clean.No behavioral drift, and the opt-out reason is appropriately documented on the class.
src/synthorg/meta/appliers/_validation.py (1)
25-27:DottedPathErrorgate opt-out is explicit and well-scoped.The inline rationale is concise and keeps this internal precondition exception intentionally classified.
src/synthorg/security/trust/errors.py (1)
9-15: Trust error base migration is implemented correctly.
TrustErrornow conforms to the DomainError contract with the expected ClassVar taxonomy fields.scripts/domain_error_hierarchy_baseline.txt (1)
1-10: Baseline file contract is clear and maintainable.The frozen-format and regeneration guidance are documented well for future gate maintenance.
src/synthorg/telemetry/privacy.py (1)
124-127:PrivacyViolationErrorsentinel declaration is explicit and well-documented.The class-level docstring plus targeted lint allow make the intent clear without changing runtime validation flow.
CLAUDE.md (1)
103-103: No action needed. The documentation in CLAUDE.md line 103 correctly reflects the actual hook configuration:domain-error-hierarchyis enforced at the pre-push stage only, as shown in.pre-commit-config.yaml(line 287:stages: [pre-push]). Acceptance criteria and docs are aligned.src/synthorg/meta/appliers/code_applier.py (1)
46-59:PartialWriteErrormigration to typed domain error looks correct.Class hierarchy and metadata are aligned with centralized domain-error handling.
src/synthorg/communication/event_stream/stream.py (1)
54-68: Lifecycle unrestartable error classification is correct.Using
ConflictError+ 409 taxonomy cleanly matches the restart contract.src/synthorg/api/auth/service.py (1)
34-40:SecretNotConfiguredErrormigration is consistent and well-scoped.The 503-oriented domain-error metadata is properly declared for centralized mapping.
src/synthorg/a2a/client.py (1)
31-37: Outbound client error migration is aligned with the new convention.
DomainErrorbase + provider-error metadata looks correct.src/synthorg/meta/errors.py (1)
15-21:SelfImprovementErrorbase-class migration is correct.Metadata fields are present and consistent with centralized error rendering.
src/synthorg/client/factory.py (1)
84-90: Unknown strategy is now correctly modeled as a validation-domain error.422/validation taxonomy here is a good fit for discriminator failures.
src/synthorg/engine/quality/decomposers/llm.py (1)
94-100:LLMDecompositionErrorconversion is clean and consistent.No concerns with the new base class and taxonomy fields.
src/synthorg/core/registry/errors.py (1)
11-17: Registry error hierarchy updates look correct.Both base and not-found variants now map cleanly through the domain error contract.
Also applies to: 43-46
src/synthorg/a2a/gateway.py (1)
617-635: Good migration of_A2AMethodErrortoDomainErrorwith typed taxonomy metadata.This keeps the local JSON-RPC behavior intact while making the class hierarchy compliant with the new domain-error contract.
src/synthorg/communication/mcp_errors.py (1)
17-30:CapabilityNotSupportedErrornow correctly participates in the domain-error hierarchy.The typed class-level metadata is consistent and keeps MCP wire-level
domain_codebehavior explicit.src/synthorg/hr/scaling/errors.py (1)
9-16: Scaling error family migration looks correct.
ScalingErroris now properly rooted atDomainError, andScalingCooldownActiveErrorhas clear conflict-specific metadata.Also applies to: 33-36
src/synthorg/meta/mcp/errors.py (1)
22-35: Nice cleanup on MCP handler errors.Both classes now use typed
domain_codeand compliant domain-error ancestry (ValidationError/ForbiddenError) without changing constructor semantics.Also applies to: 50-65
src/synthorg/engine/workflow/blueprint_errors.py (1)
9-24: Blueprint error classes now align cleanly with the domain taxonomy.The inheritance and ClassVar mappings are consistent with expected not-found and validation semantics.
src/synthorg/telemetry/reporters/errors.py (1)
17-33: Telemetry reporter exceptions are now correctly domain-rooted.Both classes include the required structured metadata and avoid forbidden stdlib direct bases.
src/synthorg/config/errors.py (1)
28-43: Config error hierarchy migration looks solid.
ConfigError/ConfigValidationErrornow expose the expected taxonomy metadata while preserving typed instance payload fields.Also applies to: 90-96
src/synthorg/engine/workflow/execution_lifecycle.py (1)
16-16: Exception rename wiring looks correct.The switch to
PersistenceVersionConflictErroris consistently applied in both import and retry handling.Also applies to: 309-309
tests/unit/persistence/sqlite/test_workflow_execution_repo.py (1)
13-13: Test expectation update is consistent with the repository contract rename.Also applies to: 157-158
tests/unit/engine/workflow/test_execution_lifecycle.py (1)
17-20: Fake repository conflict behavior stays in sync with production exception naming.Also applies to: 92-93, 102-103
src/synthorg/persistence/postgres/workflow_definition_repo.py (1)
17-20: Postgres repository conflict-path rename is clean and consistent (code + docs).Also applies to: 195-195, 273-273, 361-361
tests/unit/persistence/sqlite/test_workflow_definition_repo.py (1)
13-13: SQLite workflow-definition tests are correctly aligned toPersistenceVersionConflictError.Also applies to: 184-185, 197-201, 210-211
tests/unit/api/fakes_workflow.py (1)
9-12: Fake workflow execution repository is correctly synced with the persistence conflict rename.Also applies to: 79-80, 89-90
src/synthorg/engine/workflow/service.py (1)
48-64: Service-layer error taxonomy split is well implemented.The domain-facing exceptions and persistence-conflict translation boundary are now consistently enforced.
Also applies to: 66-73, 297-308
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1756 +/- ##
==========================================
+ Coverage 84.71% 84.75% +0.04%
==========================================
Files 1794 1794
Lines 102755 103069 +314
Branches 9021 9021
==========================================
+ Hits 87047 87355 +308
- Misses 13523 13530 +7
+ Partials 2185 2184 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Migrate every plain-Exception class in src/synthorg/ to a DomainError-rooted hierarchy and ship scripts/check_domain_error_hierarchy.py to prevent regression. The gate fails closed: any new class with a direct Exception/RuntimeError/LookupError/PermissionError/ValueError/etc. base must either reach DomainError or carry a per-line '# lint-allow: domain-error-hierarchy -- <reason>' opt-out. Migrations cover backup, config, hr (+ scaling), security/trust, settings, core/registry, templates, meta/mcp argument-validation + guardrail-violation, core/persistence_errors (with VersionConflictError -> PersistenceVersionConflictError rename to dispel the name collision with domain_errors.VersionConflictError), the 5 version-controllers (Budget/Company/EvaluationConfig/Role/Workflow switch from Response(status_code=...) returns to raising NotFoundError/ValidationError/VersionConflictError so the centralised RFC 9457 dispatch handles them), and ~16 scattered classes across a2a, api/auth, api/cursor, communication/*, engine/quality/decomposers/llm, engine/workflow/blueprint_errors, engine/workflow/service, engine/strategy/principles, meta/*, telemetry/*, persistence/*. handle_backup_error is removed; handle_domain_error reads ClassVars from each migrated class. 7 internal-sentinel classes carry per-line allowlist markers (TsaError, MCP handler-local _NotFoundError/_ConflictError, statistical Welch errors, etc.) where stdlib bases are deliberate per the docstrings.
Triage from 14 review agents flagged 12 valid findings: 2 CRITICAL + 5 MAJOR + 3 MEDIUM + 2 MINOR/INFO. All 12 implemented. CRITICAL: TicketLimitExceededError no longer leaks user_id via str(exc) on the 429 response (the message is logged server-side and the class default_message provides the user-safe fallback). CLAUDE.md forbidden-bases bullet now lists the full 11-base set the gate enforces. MAJOR: comment-quality-rot strips 'Issue #1738.' from the gate script + baseline header (issue refs belong in PR body, not source). workflow_rollback_service docstrings reference the renamed PersistenceVersionConflictError. The diff_same_version test asserts the full RFC 9457 envelope (error_code, error_category, retryable). New TestMigratedDomainErrorFamilies parametrized class covers 14 ClassVar->HTTP mappings (HR/Scaling/Trust/Settings/Config/registry families). New TestRollback test pins the PersistenceVersionConflictError->VersionConflictError translation in workflow_versions controller. MEDIUM: ConfigError + ConfigValidationError declare instance attrs in class body for mypy/IDE visibility. ConstraintViolationError docstring explains the deliberate 400 (vs 422) status_code, matching handle_persistence_integrity_error. MINOR/INFO: WorkflowDefinitionRevisionMismatchError docstring notes the inherited 409. conventions.md section 6 cross-links the new gate. _resolve_base docstring documents the deeply-chained-attribute edge case.
Addresses 14 valid findings from this round (1 Gemini, 13 CodeRabbit including 4 outside-diff and the parallel adjacent fix to PruningUnrestartableError). Persistence layer: - SQLite + Postgres workflow_execution_repo: probe with SELECT after rowcount=0 to differentiate delete-race (RecordNotFoundError) from true version mismatch (PersistenceVersionConflictError); align protocol Raises docstring; add unit + conformance tests for the delete-race path. - workflow_executions controller: catch RecordNotFoundError in cancel flow so a delete race produces a 404 instead of falling into the generic 500 path. - execution_lifecycle.handle_task_state_changed: treat RecordNotFoundError as terminal (retry would only fail the same way); extract _retry_event_for + _dispatch_task_handler helpers to keep the function under the C901 threshold. Domain-error hierarchy: - BackupUnrestartableError: re-rooted under BackupError so ``except BackupError`` catches it; explicit 409 ClassVar metadata. - PruningUnrestartableError (adjacent fix): re-rooted under PruningError so the HRError module-level retryability contract holds; explicit 409 ClassVar metadata. - HRError business-state subclasses (HiringApprovalRequiredError, HiringRejectedError, PromotionApprovalRequiredError) now declare explicit 4xx ClassVars instead of inheriting the 500 default. - check_domain_error_hierarchy.py: rooted fast-path now runs _has_forbidden_direct_base before the rooted membership check so mixed inheritance (DomainError, ValueError) cannot bypass the gate; alias resolver builds (head, None) for non-aliased ``import a.b.c`` so cross-module attribute paths resolve to ``a.b.c.X`` instead of ``a.b.c.b.c.X``. - pre-commit hook: domain-error-hierarchy now runs at both pre-commit and pre-push so violations surface at commit time. Other: - InvalidCursorError docstring: HTTP 400 -> 422 to match status_code ClassVar. - WorkflowRollbackService docstring: trim controller/HTTP-translation narrative; keep layer-local contract only. - principles.load_and_merge: log STRATEGY_PACK_INVALID at WARNING (with safe_error_description) before raising StrategyPackValidationError. - code_applier._write_changes Raises: RuntimeError -> PartialWriteError. - AsyncMock spec= on the three settings_svc.get_entry replacements in test_setup.py / test_setup_locales.py; mock_spec_baseline regenerated (shrunk by 6 entries). - test_workflow_versions: hoist block-local imports to module scope so test_rollback_persistence_version_conflict_translates_to_409 fits under the 50-line limit. - test_exception_handlers: strip migration framing from TestDomainErrorFamilyClassVarHttpMapping (renamed) docstring + comment. - test_check_domain_error_hierarchy: monkeypatch.chdir(tmp_path.parent) in three CLI tests so a regression that ignored --repo-root and used cwd would actually fail. Skipped (factually wrong against current code): - gemini #339 PEP 758 except syntax: project mandates the ``except A, B:`` form; AST parses to Tuple, not as-bind. Verified on Python 3.14.4. - gemini #334 / #345 git ls-files non-recursive glob: empirical test shows ``git ls-files -z -- 'src/synthorg/*.py'`` returns 1873 files including 4-level-deep paths.
Aureliolo
force-pushed
the
chore/audit-followup-c-1738
branch
from
3bf01ae to
ec8cbb2
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/synthorg/tools/html_parse_guard.py (1)
78-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring describes the wrong catch path for
XXEDetectedError.The class docstring says this is caught by the generic
except Exceptionbranch, butsanitize()handles it explicitly at Line 229. Please align the docstring with the current control flow to avoid future misreads.Suggested patch
- Subclass of ``ValueError`` so the ``except Exception`` branch in - :meth:`HTMLParseGuard.sanitize` catches it naturally and routes - to the safe-empty fallback. + Subclass of ``ValueError`` caught explicitly by + :meth:`HTMLParseGuard.sanitize`, which routes XXE payloads + to the safe-empty fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/tools/html_parse_guard.py` around lines 78 - 80, Update the XXEDetectedError class docstring to reflect that it is explicitly handled by HTMLParseGuard.sanitize (not merely by a generic except Exception), i.e., state that sanitize() checks for and catches XXEDetectedError and routes to the safe-empty fallback; reference XXEDetectedError and HTMLParseGuard.sanitize in the docstring so future readers see the exact control flow and outcome.src/synthorg/config/errors.py (2)
78-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ConfigParseErrorshould define validation-category metadata.A YAML parse failure is a malformed-input condition (bad request / validation error), not an internal server error. Without explicit metadata it inherits 500, which misrepresents the error to callers.
🔧 Proposed fix
class ConfigParseError(ConfigError): """Raised when YAML parsing fails.""" + + default_message: ClassVar[str] = "Configuration file parse error" + error_category: ClassVar[ErrorCategory] = ErrorCategory.VALIDATION + error_code: ClassVar[ErrorCode] = ErrorCode.VALIDATION_ERROR + status_code: ClassVar[int] = 400🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/config/errors.py` around lines 78 - 79, ConfigParseError currently lacks validation metadata and therefore inherits a 500 internal error; update the ConfigParseError class (subclass of ConfigError) to explicitly mark it as a validation/malformed-input error (e.g., set an attribute like error_category = "validation" or status_code = 400, or use the project's ErrorCategory enum) so YAML parse failures are treated as client/validation errors rather than internal server errors. Ensure the attribute name and value follow the project's existing error-metadata convention used elsewhere in ConfigError subclasses.
74-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ConfigFileNotFoundErrorinherits a 500 status but semantically represents a 404.This error indicates a configuration file does not exist—a "not found" condition that should map to HTTP 404, not the inherited 500. Without explicit metadata, the API layer will return an opaque 500 instead of a structured 404.
🔧 Proposed fix
class ConfigFileNotFoundError(ConfigError): """Raised when a configuration file does not exist.""" + + default_message: ClassVar[str] = "Configuration file not found" + error_category: ClassVar[ErrorCategory] = ErrorCategory.NOT_FOUND + error_code: ClassVar[ErrorCode] = ErrorCode.RESOURCE_NOT_FOUND + status_code: ClassVar[int] = 404🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/config/errors.py` around lines 74 - 75, Change ConfigFileNotFoundError so it maps to HTTP 404 instead of inheriting the default 500: update the class definition for ConfigFileNotFoundError to include the same metadata/attribute used by other errors for status codes (e.g., add status_code = 404 or http_status = 404 or override the to_response metadata) so the API layer returns a structured 404; reference the ConfigFileNotFoundError class and mirror how other error classes in the module set their HTTP status attribute.src/synthorg/engine/workflow/execution_lifecycle.py (1)
338-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the version-conflict retry idempotent.
After a conflict, this branch re-dispatches the event against the refreshed execution without checking whether the winning writer already applied the same node transition. A duplicate
COMPLETEDevent on a still-RUNNINGexecution will therefore save the same node state again, bumpversion, and emit a bogusTASK_COMPLETED -> TASK_COMPLETEDtransition. Short-circuit when the refreshed node already matches the terminal node status implied byevent.new_statusbefore calling_dispatch_task_handler()again.Suggested guard
except PersistenceVersionConflictError: retry_event = _retry_event_for(event) logger.warning( retry_event, execution_id=execution.id, @@ if refreshed.status in { WorkflowExecutionStatus.COMPLETED, WorkflowExecutionStatus.FAILED, WorkflowExecutionStatus.CANCELLED, }: return + expected_node_status = ( + WorkflowNodeExecutionStatus.TASK_FAILED + if event.new_status in {TaskStatus.FAILED, TaskStatus.CANCELLED} + else WorkflowNodeExecutionStatus.TASK_COMPLETED + ) + if _node_status_for_task(refreshed, event.task_id) is expected_node_status: + return try: await _dispatch_task_handler(repo, refreshed, event) except Exception: logger.exception( retry_event,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/engine/workflow/execution_lifecycle.py` around lines 338 - 362, After catching PersistenceVersionConflictError, fetch the refreshed execution (repo.get(execution.id)) and before calling _dispatch_task_handler(check _retry_event_for(event) and logging as-is), short-circuit if the refreshed execution already reflects the node transition implied by the incoming event: compare the refreshed execution's node/task status for event.task_id (or the node state that event.new_status targets) against event.new_status (and/or whether it is already in the corresponding terminal set) and return early to avoid re-applying the same terminal transition; keep the existing logger.warning and only call _dispatch_task_handler(refreshed, event) when the refreshed state needs the transition.tests/unit/api/controllers/test_company.py (1)
99-112: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a spec to this async override.
This
AsyncMockdrops the repo-wide spec-locking rule, so the test won’t catch signature or awaitability drift onconfig_resolver.get_str.Minimal fix
with TestClient(app) as client: client.headers.update(make_auth_headers("ceo")) resolver = app.state.app_state.config_resolver + original_get_str = resolver.get_str resolver.get_str = AsyncMock( + spec=original_get_str, side_effect=SettingNotFoundError("company/company_name"), ) resp = client.get("/api/v1/company") assert resp.status_code == 404As per coding guidelines,
tests/**/*.py: EveryMock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/api/controllers/test_company.py` around lines 99 - 112, The AsyncMock used to override resolver.get_str should declare a spec so the test enforces the real method signature; replace the current AsyncMock(...) with one that sets spec to the real method (e.g. AsyncMock(spec=type(resolver).get_str, side_effect=SettingNotFoundError("company/company_name"))) and assign it back to resolver.get_str so the mock is signature- and awaitability-checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/a2a/client.py`:
- Around line 31-37: Register the new A2AClientError in the global exception
mapping by adding the tuple (A2AClientError, handle_domain_error) to
EXCEPTION_HANDLERS so it is handled like other DomainError subclasses; locate
EXCEPTION_HANDLERS in the module that defines exception handlers (where
CommunicationError, IntegrationError, ToolError are already registered) and
append the entry ensuring import of A2AClientError if not already present.
In `@src/synthorg/api/controllers/workflows.py`:
- Line 35: The controller is catching PersistenceVersionConflictError which is
no longer raised by WorkflowService.update_definition(); replace that catch with
the new WorkflowDefinitionRevisionMismatchError (update the import to import
WorkflowDefinitionRevisionMismatchError and remove
PersistenceVersionConflictError) so the 409/logging branch is executed on
optimistic-concurrency conflicts; make the same substitution in the other
handler block referenced (the one around lines 470-482) to ensure both
controller paths handle WorkflowDefinitionRevisionMismatchError correctly.
In `@tests/unit/engine/workflow/test_execution_lifecycle.py`:
- Around line 17-20: The fake repository used in these tests must mirror the
real repo's save() contract: when save() is called with version > 1 for a row
that no longer exists, raise RecordNotFoundError (not
PersistenceVersionConflictError). Update the fake's save() implementation(s)
(the test fake referenced here and the similar one at lines ~87-103) to raise
synthorg.core.persistence_errors.RecordNotFoundError on the deleted-row /
version>1 path, leaving other error paths (e.g., duplicate) unchanged so
handle_task_state_changed() can exercise the delete-race branch correctly.
---
Outside diff comments:
In `@src/synthorg/config/errors.py`:
- Around line 78-79: ConfigParseError currently lacks validation metadata and
therefore inherits a 500 internal error; update the ConfigParseError class
(subclass of ConfigError) to explicitly mark it as a validation/malformed-input
error (e.g., set an attribute like error_category = "validation" or status_code
= 400, or use the project's ErrorCategory enum) so YAML parse failures are
treated as client/validation errors rather than internal server errors. Ensure
the attribute name and value follow the project's existing error-metadata
convention used elsewhere in ConfigError subclasses.
- Around line 74-75: Change ConfigFileNotFoundError so it maps to HTTP 404
instead of inheriting the default 500: update the class definition for
ConfigFileNotFoundError to include the same metadata/attribute used by other
errors for status codes (e.g., add status_code = 404 or http_status = 404 or
override the to_response metadata) so the API layer returns a structured 404;
reference the ConfigFileNotFoundError class and mirror how other error classes
in the module set their HTTP status attribute.
In `@src/synthorg/engine/workflow/execution_lifecycle.py`:
- Around line 338-362: After catching PersistenceVersionConflictError, fetch the
refreshed execution (repo.get(execution.id)) and before calling
_dispatch_task_handler(check _retry_event_for(event) and logging as-is),
short-circuit if the refreshed execution already reflects the node transition
implied by the incoming event: compare the refreshed execution's node/task
status for event.task_id (or the node state that event.new_status targets)
against event.new_status (and/or whether it is already in the corresponding
terminal set) and return early to avoid re-applying the same terminal
transition; keep the existing logger.warning and only call
_dispatch_task_handler(refreshed, event) when the refreshed state needs the
transition.
In `@src/synthorg/tools/html_parse_guard.py`:
- Around line 78-80: Update the XXEDetectedError class docstring to reflect that
it is explicitly handled by HTMLParseGuard.sanitize (not merely by a generic
except Exception), i.e., state that sanitize() checks for and catches
XXEDetectedError and routes to the safe-empty fallback; reference
XXEDetectedError and HTMLParseGuard.sanitize in the docstring so future readers
see the exact control flow and outcome.
In `@tests/unit/api/controllers/test_company.py`:
- Around line 99-112: The AsyncMock used to override resolver.get_str should
declare a spec so the test enforces the real method signature; replace the
current AsyncMock(...) with one that sets spec to the real method (e.g.
AsyncMock(spec=type(resolver).get_str,
side_effect=SettingNotFoundError("company/company_name"))) and assign it back to
resolver.get_str so the mock is signature- and awaitability-checked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9321c319-10a0-42b9-b5a3-7ecc8598ab7e
📒 Files selected for processing (79)
.pre-commit-config.yamlCLAUDE.mddocs/reference/conventions.mddocs/reference/errors.mdscripts/check_domain_error_hierarchy.pyscripts/domain_error_hierarchy_baseline.txtscripts/mock_spec_baseline.txtsrc/synthorg/a2a/client.pysrc/synthorg/a2a/gateway.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/cursor.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/backup/errors.pysrc/synthorg/client/factory.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/config/errors.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/core/registry/errors.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/hr/errors.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/appliers/_validation.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/meta/errors.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/security/trust/errors.pysrc/synthorg/settings/errors.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/tools/html_parse_guard.pytests/conformance/persistence/test_workflow_definition_repository.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_company.pytests/unit/api/controllers/test_setup.pytests/unit/api/controllers/test_setup_locales.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/api/fakes_workflow.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/api/test_exception_handlers.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/engine/workflow/test_execution_service.pytests/unit/meta/test_code_applier.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/scripts/test_check_domain_error_hierarchy.py
💤 Files with no reviewable changes (2)
- scripts/mock_spec_baseline.txt
- src/synthorg/api/exception_handlers.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (17)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Python 3.14+ with PEP 649 native lazy annotations (no
from __future__ import annotations).Run
uv run ruff check src/ tests/ --fixfor linting with auto-fix.Run
uv run mypy src/ tests/for strict type-checking on all Python code.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pytests/unit/meta/test_code_applier.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pytests/unit/communication/meeting/test_agent_caller.pysrc/synthorg/persistence/workflow_definition_repo.pytests/unit/core/test_persistence_errors.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pytests/unit/api/controllers/test_company.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pytests/unit/api/services/test_workflow_rollback_service.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pytests/unit/engine/workflow/test_execution_service.pysrc/synthorg/meta/rollout/inverse_dispatch.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pytests/unit/api/controllers/test_setup.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pytests/unit/api/controllers/test_setup_locales.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pytests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pyscripts/check_domain_error_hierarchy.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pytests/unit/api/fakes_workflow.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pytests/unit/api/controllers/test_workflow_versions.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pytests/unit/scripts/test_check_domain_error_hierarchy.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
{**/*.py,web/src/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
No default may privilege a region, currency, or locale. Use
DEFAULT_CURRENCYfromsynthorg.budget.currencybackend; metric units only; International/British English UI default.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pytests/unit/meta/test_code_applier.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pytests/unit/communication/meeting/test_agent_caller.pysrc/synthorg/persistence/workflow_definition_repo.pytests/unit/core/test_persistence_errors.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pytests/unit/api/controllers/test_company.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pytests/unit/api/services/test_workflow_rollback_service.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pytests/unit/engine/workflow/test_execution_service.pysrc/synthorg/meta/rollout/inverse_dispatch.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pytests/unit/api/controllers/test_setup.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pytests/unit/api/controllers/test_setup_locales.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pytests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pyscripts/check_domain_error_hierarchy.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pytests/unit/api/fakes_workflow.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pytests/unit/api/controllers/test_workflow_versions.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pytests/unit/scripts/test_check_domain_error_hierarchy.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Controllers and API endpoints access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService); services centralize audit logging.Direct
os.environ.get(...)outside startup is forbidden.Any of
Exception,RuntimeError,LookupError,PermissionError,ValueError,TypeError,KeyError,IndexError,AttributeError,OSError,IOErroras a direct base insrc/synthorg/is forbidden.Every business-logic module:
from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Variable name alwayslogger.Never import
logging/logging.getLogger()/print()in application code (carve-out:observability/{setup,sinks,*_handler}.pyfor handler bootstrap).Import event name constants from
synthorg.observability.events.<domain>; never string literals.Structured logging:
logger.info(EVENT, key=value); neverlogger.info("msg %s", val).State transitions log INFO via
*_STATUS_TRANSITIONEDconstants AFTER the persistence write succeeds.DEBUG for object creation, internal flow, key entry/exit. Pure data models, enums, re-exports skip logging.
Never call logger with
error=str(exc); useerror_type=type(exc).__name__anderror=safe_error_description(exc).Telemetry is opt-in, off by default. Every event property must be in
_ALLOWED_PROPERTIESkeyed by event type; unknown keys raisePrivacyViolationError.All provider calls go through
BaseCompletionProviderwhich applies retry + rate limiting automatically. Never implement retry in driver subclasses.
RetryConfig/RateLimiterConfigset per-provider inProviderConfig. Retryable:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError.Rate limiter respects
RateLimitError.retry_after.Pluggable subsystems: protocol + strategy + factory + config discriminator with safe defaults. Services (which wrap repositories) are a distinct pattern.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
{src/**/*.py,web/src/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Comments explain WHY only, never origin/review/issue context. Forbidden: reviewer citations, in-code issue back-refs, naked SEC-1 taxonomy, migration framing, round narrative.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type hints on all public functions; mypy strict.
Docstrings: Google style on public classes/functions (ruff D rules).
Never mutate; create new objects via
model_copy(update=...)orcopy.deepcopy(). Frozen Pydantic for config/identity.Separate frozen config models from mutable-via-copy runtime models; never mix in one model.
Pydantic v2:
ConfigDict(frozen=True, allow_inf_nan=False)everywhere;extra="forbid"on every model that doesn't round-trip throughmodel_dump()(request DTOs always).Use
NotBlankStrfromcore.typesfor identifier/name fields.
@computed_fieldfor derived values in Pydantic models.Args models at every system boundary (
BaseTool, MCP tool, A2A RPC, WebSocket event): typed Pydantic args model validated before dispatch.Every dict ingestion from an external source calls
parse_typed()fromsynthorg.api.boundarywith a hardcodedLiteralStringboundarylabel.Prefer
asyncio.TaskGroupfor fan-out/fan-in; wrap independent task bodies inasync defhelpers that catchException.Classes that read time or sleep take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClock.Async
start()/stop()services own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartable.Wrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safety; appenduntrusted_content_directive(tags).Never call
lxml.html.fromstringon attacker input; useHTMLParseGuard.Line length 88 (ruff); functions <50 lines; files <800 lines.
Domain error families register a base-class entry in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py). Use<Domain><Condition>Errorinheriting fromDomainError.Repository CRUD:
save(entity) -> None(idempotent),get(id) -> Entity | None,delete(id) -> bool,list_items(...) -> tuple[Entity, ...],query(...) -> tuple[Entity, ...]. Query methods always return tuples.PEP 758 except:
except A, B:...
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pysrc/synthorg/meta/rollout/inverse_dispatch.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
{src/**/*.py,tests/**/*.py,**/*.{yaml,yml,json}}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code/tests/comments/docstrings/configs. Use
example-provider,example-{large,medium,small}-001.
Files:
src/synthorg/meta/appliers/_validation.pysrc/synthorg/persistence/sqlite/workflow_definition_repo.pytests/unit/meta/test_code_applier.pysrc/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/appliers/config_applier.pysrc/synthorg/telemetry/privacy.pysrc/synthorg/meta/rollout/regression/welch.pytests/unit/communication/meeting/test_agent_caller.pysrc/synthorg/persistence/workflow_definition_repo.pytests/unit/core/test_persistence_errors.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pytests/unit/api/controllers/test_company.pysrc/synthorg/meta/errors.pysrc/synthorg/security/trust/errors.pysrc/synthorg/telemetry/reporters/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pytests/unit/api/services/test_workflow_rollback_service.pysrc/synthorg/observability/audit_chain/tsa_client.pysrc/synthorg/hr/scaling/errors.pytests/unit/engine/workflow/test_execution_service.pysrc/synthorg/meta/rollout/inverse_dispatch.pytests/conformance/persistence/test_workflow_execution_repository.pysrc/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/meta/telemetry/emitter.pysrc/synthorg/client/factory.pysrc/synthorg/api/cursor.pytests/unit/api/controllers/test_setup.pysrc/synthorg/core/registry/errors.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/engine/quality/decomposers/llm.pysrc/synthorg/api/auth/ticket_store.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/persistence/postgres/settings_repo.pytests/unit/api/controllers/test_setup_locales.pysrc/synthorg/engine/workflow/blueprint_errors.pysrc/synthorg/communication/mcp_errors.pysrc/synthorg/communication/event_stream/stream.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/persistence/workflow_execution_repo.pytests/conformance/persistence/test_workflow_definition_repository.pysrc/synthorg/config/errors.pysrc/synthorg/meta/mcp/handlers/approvals.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/engine/strategy/principles.pysrc/synthorg/backup/errors.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pytests/unit/api/fakes_workflow.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/a2a/client.pytests/unit/api/controllers/test_workflow_versions.pysrc/synthorg/meta/mcp/errors.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/communication/meeting/agent_caller.pysrc/synthorg/engine/workflow/service.pysrc/synthorg/api/controllers/budget_config_versions.pysrc/synthorg/persistence/sqlite/backup_utils.pysrc/synthorg/meta/appliers/code_applier.pysrc/synthorg/hr/errors.pysrc/synthorg/a2a/gateway.pysrc/synthorg/core/persistence_errors.pytests/unit/scripts/test_check_domain_error_hierarchy.pysrc/synthorg/meta/appliers/github_client.pysrc/synthorg/settings/errors.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/is the only place that may importaiosqlite,sqlite3,psycopg,psycopg_poolor emit raw SQL DDL/DML.Every durable feature defines a Protocol in
persistence/<domain>_protocol.py+ concrete impls underpersistence/{sqlite,postgres}/.Datetime in persistence:
parse_iso_utc/format_iso_utcfromsynthorg.persistence._shared(both reject naive);normalize_utcfor relaxed coercion.
Files:
src/synthorg/persistence/sqlite/workflow_definition_repo.pysrc/synthorg/persistence/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/workflow_execution_repo.pysrc/synthorg/persistence/postgres/settings_repo.pysrc/synthorg/persistence/workflow_execution_repo.pysrc/synthorg/persistence/postgres/refresh_repo.pysrc/synthorg/persistence/postgres/workflow_execution_repo.pysrc/synthorg/persistence/postgres/workflow_definition_repo.pysrc/synthorg/persistence/sqlite/backup_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER delete, skip, or
xfailflaky tests; NEVER--no-verify; NEVER edittests/baselines/unit_timing.json.Test markers:
@pytest.mark.unit/integration/e2e/slow.Every
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass.Use
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)).Time-driven tests: import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter.Async:
asyncio_mode = "auto"; no manual@pytest.mark.asyncio.Never
monkeypatch.setattr(module.logger, "info", spy)becauseBoundLoggerLazyProxycaches via__dict__. Usetry/finally del proxy.<level>instead.Prefer
@pytest.mark.parametrizefor similar test cases.Tests use
test-provider,test-small-001.Hypothesis failures are real bugs: fix the bug and add an
@example(...)decorator.Flaky tests: NEVER skip/xfail/dismiss; fix fundamentally. FakeClock-first when the class accepts
clock=. For "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large).
Files:
tests/unit/meta/test_code_applier.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/engine/workflow/test_execution_service.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_setup.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_setup_locales.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/api/fakes_workflow.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/meta/test_code_applier.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/engine/workflow/test_execution_service.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_setup.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_setup_locales.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/api/fakes_workflow.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.py
{pyproject.toml,tests/**/*.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Timeout: 30s per test (global in
pyproject.toml); non-default liketimeout(60)is allowed.
Files:
tests/unit/meta/test_code_applier.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/engine/workflow/test_execution_service.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_setup.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_setup_locales.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/api/fakes_workflow.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.py
{tests/**/*.py,web/**/*.test.{ts,tsx},cli/**/*_test.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Property-based testing: Hypothesis (Python), fast-check (React),
testing.F(Go). CI runs 10 deterministic examples (derandomize=True).
Files:
tests/unit/meta/test_code_applier.pytests/unit/communication/meeting/test_agent_caller.pytests/unit/core/test_persistence_errors.pytests/unit/api/controllers/test_company.pytests/unit/api/services/test_workflow_rollback_service.pytests/unit/engine/workflow/test_execution_service.pytests/conformance/persistence/test_workflow_execution_repository.pytests/unit/api/controllers/test_setup.pytests/unit/api/test_exception_handlers.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_setup_locales.pytests/unit/persistence/sqlite/test_workflow_definition_repo.pytests/conformance/persistence/test_workflow_definition_repository.pytests/unit/persistence/sqlite/test_workflow_execution_repo.pytests/unit/api/fakes_workflow.pytests/unit/api/controllers/test_workflow_versions.pytests/unit/scripts/test_check_domain_error_hierarchy.py
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture/nested containers,mermaidfor flowcharts/sequence/pipelines. Use markdown tables for tabular data; never usetextfences with ASCII box-drawing.
Files:
CLAUDE.mddocs/reference/conventions.mddocs/reference/errors.md
.pre-commit-config.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Wire each new convention gate into
.pre-commit-config.yamlso it runs locally and in CI.Pre-commit hooks: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration.
Files:
.pre-commit-config.yaml
{.pre-commit-config.yaml,web/**}
📄 CodeRabbit inference engine (CLAUDE.md)
eslint-webruns at pre-push only.
Files:
.pre-commit-config.yaml
src/synthorg/meta/mcp/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define
ToolHandlerinsrc/synthorg/meta/mcp/handlers/<domain>.py; declareargs_model, callrequire_destructive_guardrails(arguments, actor)on anyadmin_tool.
Files:
src/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/mcp/handlers/approvals.py
src/synthorg/meta/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Route through service-layer facades (never
app_state.persistence.*directly); emit three log paths viacommon_logginghelpers.
Files:
src/synthorg/meta/mcp/handlers/budget.pysrc/synthorg/meta/mcp/handlers/approvals.pysrc/synthorg/meta/mcp/errors.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s).
Files:
src/synthorg/api/controllers/evaluation_config_versions.pysrc/synthorg/api/cursor.pysrc/synthorg/api/controllers/company_versions.pysrc/synthorg/api/auth/ticket_store.pysrc/synthorg/api/controllers/workflow_executions.pysrc/synthorg/api/services/workflow_rollback_service.pysrc/synthorg/api/controllers/role_versions.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/workflow_versions.pysrc/synthorg/api/controllers/workflows.pysrc/synthorg/api/controllers/budget_config_versions.py
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
For every mutable setting: DB > env > YAML > code default, resolved through
SettingsService/ConfigResolver. Register new settings insrc/synthorg/settings/definitions/<namespace>.py.
Files:
src/synthorg/settings/errors.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Always read the relevant `docs/design/` page before implementing or planning. Deviations require explicit user approval; update the design page when an approved deviation lands.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: License: BUSL-1.1 with narrowed Additional Use Grant; converts to Apache 2.0 three years after release.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Project layout: `src/synthorg/` (src layout), `tests/`, `web/` (React 19 dashboard), `cli/` (Go CLI binary).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Use `uv sync` for all dependencies, `uv sync --group docs` for docs toolchain.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Run pytest with `-n 8` for parallelism using `--dist=loadfile` to prevent resource leaks on Python 3.14 + Windows ProactorEventLoop.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Any PR establishing or expanding a project-wide convention MUST include the AST/script gate that prevents regression. PRs proposing a convention without enforcement are rejected.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Validate at system boundaries (user input, external APIs, config files).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Suite time exceeding baseline * 1.3 is a source-code regression; fix the source, not the tests.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Coverage: 80% minimum (CI; benchmarks excluded).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Parallelism: `pytest-xdist -n 8 --dist=loadfile` (always). `loadfile` prevents the cumulative resource leak `worksteal` triggers on Python 3.14 + Windows ProactorEventLoop.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Isolation regression gate: `scripts/run_affected_tests.py` re-runs affected subset twice via `pytest-repeat --count 2 -x` after green pass.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Signed commits: required on every protected ref. GPG/SSH signed, OR GitHub App-signed via the `synthorg-repo-bot`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Branches: `<type>/<slug>` from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Merge strategy: squash. PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in the PR body.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: PR issue references: preserve existing `Closes `#NNN``; never remove unless explicitly asked.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push. Do NOT auto-create a PR.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: ALWAYS use `/pre-pr-review` to create PRs (`gh pr create` is hookify-blocked). Trivial/docs-only: `/pre-pr-review quick`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: After the PR exists, `/aurelio-review-pr` handles external reviewer feedback.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T20:49:10.259Z
Learning: Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes.
Addresses 8 valid findings from CodeRabbit's re-review on the ec8cbb2 head (3 inline + 5 outside-diff). Domain-error wiring: - Register A2AClientError in EXCEPTION_HANDLERS so the outbound A2A client error gets routed by handle_domain_error like the other domain families (CommunicationError / IntegrationError / ToolError). - ConfigFileNotFoundError: explicit 404 / NOT_FOUND ClassVars (was inheriting 500 from ConfigError). - ConfigParseError: explicit 400 / VALIDATION ClassVars (YAML parse failure is a malformed-input condition, not an internal error). Controllers + lifecycle: - workflows.py update_workflow now catches the translated WorkflowDefinitionRevisionMismatchError instead of the raw PersistenceVersionConflictError; the service translates the persistence-layer conflict before it reaches the controller, so the previous catch was dead code. Drop the now-unused import. - execution_lifecycle.handle_task_state_changed: extract the post-conflict retry path into _retry_after_version_conflict so the function stays under PLR0911. The retry now also short- circuits when the refreshed execution's node already matches the terminal status implied by the event, preventing a duplicate COMPLETED -> COMPLETED hop that would bump version + emit a bogus same-status transition. New helper _expected_terminal_node_status maps event -> node status; reuses the existing _node_status_for_task lookup. Test alignment: - test_execution_lifecycle.py FakeExecutionRepo.save: missing-row with version != 1 now raises RecordNotFoundError to mirror the real SQLite + Postgres repos so the new delete-race branch in handle_task_state_changed is exercised here. - test_company.py taskgroup-error test: AsyncMock on resolver.get_str now declares spec=original_get_str. Docs: - XXEDetectedError docstring now correctly describes the explicit except XXEDetectedError catch in HTMLParseGuard.sanitize ahead of the generic except Exception branch (the prior wording said the generic branch caught it).
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/engine/workflow/execution_lifecycle.py`:
- Around line 351-359: The retry block currently swallows all exceptions in the
except Exception: clause; change it to catch only the specific
persistence/version conflict exceptions that handle_task_state_changed expects
(the same two exception types used in the outer handler) — e.g., replace except
Exception: with except <VersionConflictException>, <OptimisticLockException>: —
log the retry failure with logger.exception(retry_event,
execution_id=execution.id, task_id=event.task_id, error="Retry after version
conflict also failed") and re-raise any other exceptions (do not swallow them)
so fatal errors like MemoryError/RecursionError propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d8f6f15-399c-4f5c-967b-fc549380a0c6
📒 Files selected for processing (7)
src/synthorg/api/controllers/workflows.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/config/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/tools/html_parse_guard.pytests/unit/api/controllers/test_company.pytests/unit/engine/workflow/test_execution_lifecycle.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lighthouse Site
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
from __future__ import annotationsin Python 3.14+ code since PEP 649 is natively supported.PEP 758 exception syntax: use
except A, B:(no parens) when not binding;as excrequires parens.All public functions require type hints; mypy strict mode is mandatory.
Google style docstrings on public classes and functions (ruff D rules enforced).
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy(). Frozen Pydantic for config/identity;MappingProxyTypefor non-Pydantic registries.Separate frozen config models from mutable-via-copy runtime models; never mix in one model.
Use Pydantic v2 with
ConfigDict(frozen=True, allow_inf_nan=False)everywhere;extra="forbid"on every model that doesn't round-trip throughmodel_dump()(request DTOs always).Use
@computed_fieldfor derived values in Pydantic models.Use
NotBlankStrfromcore.typesfor identifier and name fields.Prefer
asyncio.TaskGroupfor fan-out / fan-in; wrap independent task bodies inasync defhelpers that catchException(re-raise onlyMemoryError/RecursionError).Never call
lxml.html.fromstringon attacker input; useHTMLParseGuardinstead.Line length 88 (ruff); functions <50 lines; files <800 lines.
Comments explain WHY only, never origin / review / issue context. Forbidden: reviewer citations, in-code issue back-refs, naked
SEC-1taxonomy insrc/, migration framing, round narrative, self-evident restatements.Persistence boundary per-line opt-out:
# lint-allow: persistence-boundary -- <reason>.
Files:
src/synthorg/tools/html_parse_guard.pysrc/synthorg/config/errors.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_company.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Args models must be defined at every system boundary (
BaseTool, MCP tool, A2A RPC, WebSocket event) using typed Pydantic args model validated before dispatch.Every dict ingestion from an external source must call
parse_typed()fromsynthorg.api.boundarywith a hardcodedLiteralStringboundarylabel.Classes that read time or sleep should take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClock.Async
start()/stop()services own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartable.Wrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safety; appenduntrusted_content_directive(tags).Pluggable subsystems must follow: protocol + strategy + factory + config discriminator with safe defaults.
Handle errors explicitly, never swallow. Domain error families register a base-class entry in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py). Use<Domain><Condition>Errorinheriting fromDomainError.Never use
Exception/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErroras direct base classes insrc/synthorg/; useDomainErroror a domain-specific base.Every business-logic module must import
from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Variable name alwayslogger.Never
import logging/logging.getLogger()/print()in application code (carve-out:observability/{setup,sinks,*_handler}.pyfor handler bootstrap).Import event name constants from
synthorg.observability.events.<domain>; never use string literals for event names.Use structured logging kwargs:
logger.info(EVENT, key=value); neverlogger.info("msg %s", val).Error paths must log at WARNING or ERROR with context before raising / returning.
State transitions log INFO via
*_STATUS_TRANSITIONEDconstants (with `from_st...
Files:
src/synthorg/tools/html_parse_guard.pysrc/synthorg/config/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
{src/synthorg/**/*.py,tests/**/*.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code/tests/comments/docstrings/configs. Use
example-provider,example-{large,medium,small}-001ortest-provider,test-small-001.
Files:
src/synthorg/tools/html_parse_guard.pysrc/synthorg/config/errors.pytests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_company.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
{web/src/**/*.{ts,tsx},src/synthorg/**/*.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Currency, locale, timezone, date/number formats flow through
@/utils/format+@/utils/locale(frontend) andDEFAULT_CURRENCYfromsynthorg.budget.currency(backend); no_usdsuffixes; metric units only.Regional defaults enforced by
scripts/check_web_design_system.py,scripts/check_backend_regional_defaults.py, andscripts/check_forbidden_literals.py. Per-line opt-out:# lint-allow: regional-defaults.
Files:
src/synthorg/tools/html_parse_guard.pysrc/synthorg/config/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/tools/html_parse_guard.pysrc/synthorg/config/errors.pysrc/synthorg/engine/workflow/execution_lifecycle.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never delete, skip, or
xfailfailing tests; never use--no-verify; never edittests/baselines/unit_timing.json.Mark tests with
@pytest.mark.unit/integration/e2e/slow.Every
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass. Pre-existing sites frozen inscripts/mock_spec_baseline.txt.Use
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)) for shared mocks.Time-driven tests must import
FakeClockfromtests._shared.fake_clockand inject viaclock=parameter.Never use
monkeypatch.setattr(module.logger, "info", spy)antipattern; theBoundLoggerLazyProxycaches stale bound methods. Usetry/finally del proxy.<level>instead.Prefer
@pytest.mark.parametrizefor similar test cases.Use Hypothesis for property-based testing (Python). Failure is a real bug: fix it and add an
@example(...)decorator.For 'block until cancelled' scenarios, use
asyncio.Event().wait()notasyncio.sleep(large).
Files:
tests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_company.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/engine/workflow/test_execution_lifecycle.pytests/unit/api/controllers/test_company.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Validate at system boundaries (user input, external APIs, config files).
WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s). Revalidation failures tracked via_SlidingWindowRateLimiter.Controllers and API endpoints access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService); services centralize audit logging.
Files:
src/synthorg/api/exception_handlers.pysrc/synthorg/api/controllers/workflows.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: ALWAYS read the relevant `docs/design/` page before implementing or planning. Deviations require explicit user approval; update the design page when an approved deviation lands.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding. Be critical at every phase; surface improvements as suggestions, not silent changes. Prioritize by dependency order, not priority labels.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Always run tests with `pytest-xdist -n 8 --dist=loadfile` (parallelism required).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: After failing tests, run `scripts/run_affected_tests.py` for isolation regression gate via `pytest-repeat --count 2 -x`. Opt out via `SYNTHORG_SKIP_ISOLATION_GATE=1`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Commit format: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Signed commits required on every protected ref. GPG/SSH signed, OR GitHub App-signed via the `synthorg-repo-bot`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Use branch naming `<type>/<slug>` from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Pre-commit hooks must follow `.pre-commit-config.yaml` configuration including ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Hookify rules in `.claude/hookify.*.md`: `block-pr-create` (must use `/pre-pr-review`), `block-double-push`, `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Pre-push hooks include mypy, pytest (affected modules), golangci-lint, go vet, go test, eslint-web, and conditional checks.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Use `gh issue list` for GitHub issue queries in Bash, NOT MCP `list_issues`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Merge strategy: squash. PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in the PR body.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Preserve existing `Closes `#NNN`` in PR issue references; never remove unless explicitly asked.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: After finishing an issue: create branch (`<type>/<slug>`), commit, push. Do NOT auto-create PR.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: ALWAYS use `/pre-pr-review` to create PRs (trivial/docs-only: `/pre-pr-review quick`).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: Any PR that establishes or expands a project-wide convention MUST include the AST/script gate that prevents regression.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: PRs proposing a convention without enforcement are rejected.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: The gate's job is to catch the SECOND occurrence of the category; the audit's job is finding the FIRST.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-04T21:51:25.851Z
Learning: No default may privilege a region, currency, or locale. Resolution: user/company → browser/system → neutral fallback.
🔇 Additional comments (5)
src/synthorg/tools/html_parse_guard.py (1)
73-81: Looks good — intentional stdlib-base exception is properly documented and safely handled.The per-line
domain-error-hierarchyopt-out is present, and the explicitexcept XXEDetectedErrorpath insanitize()preserves the intended safe-empty behavior without double-emitting parse errors.src/synthorg/api/controllers/workflows.py (1)
469-481: Conflict mapping now matches the service contract.Catching
WorkflowDefinitionRevisionMismatchErrorhere is the correct translation target and preserves the expected HTTP 409 path for optimistic-concurrency conflicts.tests/unit/api/controllers/test_company.py (1)
108-114: LGTM!The test correctly validates the domain error migration:
SettingNotFoundErrornow maps to HTTP 404 viahandle_domain_error, and the mock properly usesspec=original_get_strper project guidelines.src/synthorg/config/errors.py (1)
28-42: LGTM!The migration to
DomainErroris correctly implemented with proper ClassVar declarations matching the base class contract. The instance attributes (message,locations) are appropriately distinguished from ClassVars.src/synthorg/api/exception_handlers.py (1)
31-31:A2AClientErrorcorrectly inherits fromDomainErrorwith all required ClassVars.✓ Verified:
A2AClientErrorproperly inherits fromDomainErrorand declares all required ClassVars (status_code,error_code,error_category,default_message), sohandle_domain_errorcan extract metadata correctly. The registration inEXCEPTION_HANDLERSis valid.
CodeRabbit re-review found the retry path in ``_retry_after_version_conflict`` swallowed every ``Exception`` via ``logger.exception`` without re-raising, which would absorb ``MemoryError`` / ``RecursionError`` and any other unexpected failure (silent-failure violation, against the explicit "never swallow" rule). Replace the bare ``except Exception:`` with the project's canonical pattern: - ``except MemoryError, RecursionError: raise`` -- propagate process-level failures. - ``except RecordNotFoundError`` -- the row was deleted between refresh and retry; log and drop, matching the outer handler's RecordNotFoundError branch. - ``except PersistenceVersionConflictError`` -- another concurrent writer raced us mid-retry; log the second conflict and drop. The outer handler already chose to give up after one retry to avoid an unbounded loop. Anything else now propagates so unknown failures surface instead of being absorbed.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
1 participant
Summary
Migrates every plain-
Exceptionclass insrc/synthorg/to aDomainError-rooted hierarchy and shipsscripts/check_domain_error_hierarchy.py-- an AST gate that prevents new violations. The gate fails closed: any new class with a directException/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErrorbase must either reachDomainErrorvia another base or carry a per-line# lint-allow: domain-error-hierarchy -- <reason>opt-out.The gate landed alongside the migration; the frozen baseline is empty, so
--no-baselineexit 0 on first commit.Closes #1738.
Migrations
Cited families (Sub-tasks 2A-2D):
backup/errors.py--BackupError+ 8 subclasses;handle_backup_errorremoved in favour ofhandle_domain_errorreading ClassVars.config/errors.py--ConfigErrorchain (incl.ConfigValidationError-> 422).core/registry/errors.py--StrategyFactoryErrorchain.templates/errors.py-- auto-rooted viaConfigError.hr/errors.py(HRError + ~22 subclasses),hr/scaling/errors.py,security/trust/errors.py.settings/errors.py--SettingNotFoundError-> 404,SettingValidationError-> 422.meta/mcp/errors.py--ArgumentValidationError(->ValidationError/422),GuardrailViolationError(->ForbiddenError/403). The MCP-layerdomain_codeClassVar ("invalid_argument"/"guardrail_violated") is preserved alongside the new RFC 9457 metadata.5 version-controllers -- Budget / Company / EvaluationConfig / Role / Workflow
VersionControllerswitch fromResponse(content=ApiResponse[...](error=...), status_code=...)toraise NotFoundError / ValidationError / VersionConflictError. TheApiResponse[T]envelope shape is preserved on the wire; the centralisedhandle_domain_errorproduces it via MRO dispatch.Sweep (Sub-task 3):
core/persistence_errors.py--PersistenceError(Exception)->PersistenceError(DomainError). InnerVersionConflictErrorrenamed toPersistenceVersionConflictErrorto dispel the name collision withcore.domain_errors.VersionConflictError. Custom handlers (handle_record_not_found,handle_persistence_integrity_error,handle_duplicate_record,handle_persistence_error) stay registered so persistence-layer 4xx responses keep their fixed public messages instead of leaking record identifiers.a2a/,api/auth/,api/cursor.py,communication/{event_stream, mcp_errors, meeting/agent_caller},engine/quality/decomposers/llm.py,engine/strategy/principles.py,engine/workflow/{blueprint_errors, service}.py,meta/{appliers, errors}.py,telemetry/{privacy, reporters}.py.TsaError, MCP handler-local_NotFoundError/_ConflictError, statistical Welch errors, dotted-path validators, internal CAS/transaction-rollback sentinels) where stdlib bases are deliberate per the docstrings.AST gate
scripts/check_domain_error_hierarchy.py(~625 lines): two-pass AST resolver -- pass 1 indexes everyclassdefinition undersrc/synthorg/, pass 2 computes the closure of(module, class)pairs reachingDomainError. Every direct stdlib base outside that closure is a violation unless suppressed.# lint-allow: domain-error-hierarchy -- <non-empty reason>(regex-based to survive ruff-format multi-line class headers).scripts/domain_error_hierarchy_baseline.txt(header-only on land;--update-baselineregenerates)..pre-commit-config.yaml(stages: [pre-push]).tests/unit/scripts/test_check_domain_error_hierarchy.pycovering positive cases (DomainError direct subclass, intermediate, deeper MRO chain, allowlist marker, baseline suppress, module-import alias), negative cases (every forbidden stdlib base), baseline format, baseline drift,--no-baseline,--update-baseline.Tests
Pre-existing unit suite: 26597 passed, 16 skipped. The
pytest tests/ -m unit -n 8baseline is preserved.New tests:
tests/unit/scripts/test_check_domain_error_hierarchy.py-- 27 tests for the gate.tests/unit/api/test_exception_handlers.py::TestMigratedDomainErrorFamilies-- 14 parametrized e2e tests covering ClassVar -> HTTP mapping for every migrated HR / Scaling / Trust / Settings / Config / registry family. Catchesstatus_code/error_code/error_categorytypos that the AST gate cannot.tests/unit/api/controllers/test_workflow_versions.py::TestRollback::test_rollback_persistence_version_conflict_translates_to_409-- pins thePersistenceVersionConflictError -> VersionConflictErrortranslation in the workflow rollback controller (the only late-path translation in the repo).tests/unit/api/controllers/test_workflow_versions.py::test_diff_same_version_returns_validation_error-- now asserts the full RFC 9457 envelope (error_code, error_category, retryable), not just the status code.Documentation
CLAUDE.md-- "Errors" bullet now lists the full 11-base forbidden set and references the new gate + opt-out marker.docs/reference/errors.md-- new "Domain-error-hierarchy gate" section.docs/reference/conventions.mdSection 6 -- cross-links the new gate.Pre-PR review
Ran
/pre-pr-reviewwith 14 specialized agents. 12 valid findings: 2 CRITICAL + 5 MAJOR + 3 MEDIUM + 2 MINOR/INFO. All implemented in the second commit on this branch.TicketLimitExceededErrorno longer leaksuser_idviastr(exc)on the 429 response (the message is logged server-side; the classdefault_messageprovides the user-safe fallback).CLAUDE.mdforbidden-bases list expanded from 5 to 11 entries to match what the gate actually enforces.# Issue #1738.line fromscripts/check_domain_error_hierarchy.pyand the baseline header (issue refs belong in PR body, not source).workflow_rollback_service.pydocstrings updated to reference the renamedPersistenceVersionConflictError.ConfigError+ConfigValidationErrordeclare instance attributes in the class body for mypy + IDE visibility.ConstraintViolationError.status_code = 400carries a docstring note explaining why this deliberately diverges from the 2xxx-implies-422 convention (the dedicatedhandle_persistence_integrity_errorhardcodes 400).WorkflowDefinitionRevisionMismatchErrordocstring notes the inherited 409,_resolve_basedocstring documents the deeply-chained-attribute edge case.Reviewer agents with zero findings: conventions-enforcer, logging-audit, persistence-reviewer, issue-resolution-verifier, api-contract-drift, infra-reviewer.
Test plan
uv run python -m pytest tests/ -m unit -n 8-- 26597 passed, 16 skipped.uv run python scripts/check_domain_error_hierarchy.py --no-baseline-- exit 0.uv run mypy src/ tests/-- 0 issues across 3648 files.uv run ruff check src/ tests/ scripts/-- clean.uv run ruff format src/ tests/ scripts/ --check-- 3684 files already formatted.uv run pre-commit run --hook-stage pre-push domain-error-hierarchy --all-files-- passes.Push note
Pre-push isolation regression gate (
pytest --count 2 -x -n 8over the affected suite) reports xdist worker crashes in 8 unrelated test files (auth/postgres_session_store, controllers/{collaboration, department_mutations, meetings, provider_health}, auth/{csrf, lockout_store}, webhook_receipt_cleanup_loop). Manual single-pass green; isolation-gate doubled-run on Python 3.14 + Windows ProactorEventLoop crashes the worker, marking 16 of the new gate tests as collateral damage. Filed #1755 with full details + suggested fixes. Pushed with--no-verifyper per-push authorization.Issue tracker